Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow specifying multiple clients for run_examples #1148

Merged
merged 1 commit into from
Jun 15, 2020
Merged

Allow specifying multiple clients for run_examples #1148

merged 1 commit into from
Jun 15, 2020

Conversation

jul-sh
Copy link
Contributor

@jul-sh jul-sh commented Jun 12, 2020 •
edited
Loading

Adresses #1145

Checklist

  • Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
    • I have written tests that cover the code changes.
    • I have checked that these tests are run by
      Cloudbuild
    • I have updated documentation accordingly.
    • I have raised an issue to
      cover any TODOs and/or unfinished work.
  • Pull request includes prototype/experimental work that is under
    construction.

@jul-sh jul-sh force-pushed the run_examples_run_all_clients branch from 7fbfa48 to 92f52d1 Compare June 12, 2020 11:58
@jul-sh jul-sh marked this pull request as ready for review June 12, 2020 13:13
ipetr0v
ipetr0v approved these changes Jun 12, 2020
View reviewed changes
scripts/run_example
--root-tls-certificate="${SCRIPTS_DIR}/../examples/certs/local/ca.pem" \
"${CLIENT_ARGS[@]-}"
else
echo "The ${EXAMPLE} example does not contain a ${client_language} client. Skipping this client."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC If we will run -c rust on the example that doesn't support it, there will be no error, and the example script will return 0 as a status.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is indeed correct. It will print a warning, but it won't fail.

If we think it's important to fail in this case, we'd need to implement a slightly different approach, such as -c all to run all available clients.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the checks for which client languages exist are working properly, and the fail-open nature of the checks means that our Cloudbuild is currently running almost none of the examples:

% grep "example does not contain" log-bdcc0238-2511-4076-90da-4810b7719888.txt  | grep -v ': +'
Step #9 - "run_examples": The abitest example does not contain a rust client. Skipping this client.
Step #9 - "run_examples": The abitest example does not contain a cpp client. Skipping this client.
Step #9 - "run_examples": The abitest example does not contain a nodejs client. Skipping this client.
Step #9 - "run_examples": The translator example does not contain a rust client. Skipping this client.
Step #9 - "run_examples": The translator example does not contain a cpp client. Skipping this client.
Step #9 - "run_examples": The translator example does not contain a nodejs client. Skipping this client.
Step #9 - "run_examples": The trusted_information_retrieval example does not contain a rust client. Skipping this client.
Step #9 - "run_examples": The trusted_information_retrieval example does not contain a cpp client. Skipping this client.
Step #9 - "run_examples": The trusted_information_retrieval example does not contain a nodejs client. Skipping this client.
Step #9 - "run_examples": The hello_world example does not contain a rust client. Skipping this client.
Step #9 - "run_examples": The hello_world example does not contain a cpp client. Skipping this client.
Step #9 - "run_examples": The chat example does not contain a rust client. Skipping this client.
Step #9 - "run_examples": The chat example does not contain a cpp client. Skipping this client.
Step #9 - "run_examples": The chat example does not contain a nodejs client. Skipping this client.
Step #9 - "run_examples": The running_average example does not contain a rust client. Skipping this client.
Step #9 - "run_examples": The running_average example does not contain a cpp client. Skipping this client.
Step #9 - "run_examples": The running_average example does not contain a nodejs client. Skipping this client.
Step #9 - "run_examples": The private_set_intersection example does not contain a rust client. Skipping this client.
Step #9 - "run_examples": The private_set_intersection example does not contain a cpp client. Skipping this client.
Step #9 - "run_examples": The private_set_intersection example does not contain a nodejs client. Skipping this client.
Step #9 - "run_examples": The machine_learning example does not contain a rust client. Skipping this client.
Step #9 - "run_examples": The machine_learning example does not contain a cpp client. Skipping this client.
Step #9 - "run_examples": The machine_learning example does not contain a nodejs client. Skipping this client.
Step #9 - "run_examples": The aggregator example does not contain a rust client. Skipping this client.
Step #9 - "run_examples": The aggregator example does not contain a cpp client. Skipping this client.
Step #9 - "run_examples": The aggregator example does not contain a nodejs client. Skipping this client.
Step #10 - "run_examples_cpp": The hello_world example does not contain a cpp client. Skipping this client.
Step #10 - "run_examples_cpp": The tensorflow example does not contain a cpp client. Skipping this client.

@juliettepretot please could you take a look?

scripts/run_examples
@@ -38,12 +46,12 @@ done
examples="$(find examples -mindepth 2 -maxdepth 4 -type d -regex '.*/module.*/'"${application_language}"'$' | cut -d'/' -f2 | uniq)"
for example in ${examples}; do
if [[ "${example}" == 'aggregator' ]]; then
"${SCRIPTS_DIR}/run_example" -s "${server}" -a "${application_language}" -e aggregator -- --bucket=test --data=1:10,2:20,3:30
"${SCRIPTS_DIR}/run_example" -s "${server}" -a "${application_language}" -c "${client_languages}" -e aggregator -- --bucket=test --data=1:10,2:20,3:30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this point we might have to implement all lang clients for all examples, since example folders are often exhaustive - so developers will have less time figuring out how to work with Oak.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe yes.

cloudbuild.yaml
@@ -118,7 +100,7 @@ steps:
# Run clang-tidy after the examples finish running, since they share the same `output_base`.
- name: 'gcr.io/oak-ci/oak:latest'
id: run_clang_tidy
waitFor: ['run_example_hello_world_nodejs']
waitFor: ['run_examples_cpp']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be called run_examples_with_cpp_app, since it may be confusing what cpp stands for.

@jul-sh jul-sh merged commit 8019d83 into project-oak:master Jun 15, 2020
@jul-sh jul-sh deleted the run_examples_run_all_clients branch June 15, 2020 13:31
@github-actions
Copy link

Reproducibility Index:

2914fe02bc3443f6131dbc1162c8192a5d4e0864ff7c36f01586cb9bade8e63a  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
b1b5608763abc792b0a24e044e14e5917609f8a5bbdf1c67882d0c4324070946  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
25d140732267a21be7b102ff81fb7f60de74b73ab598ca94ae7fd0fa09c28355  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
bc61f670b87e9a96a66e5e9efcd56c97cb590dee9ae8db6b236b372c093c2583  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
903d9ce0f2a7bdcab33e240c9a843095fd733684d1352eb6c81bdcfc04642481  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
0bd3b7a2c3f9e2c7d411cc32b01dce9e21fe792528de385903d5bd7e1de44f60  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
9ec622e6689d0600e655e5dfd3ac30dddd3c9d85341fca343f84ccae73c73f98  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
090482036c29de804768c0b6bb8389883308b6c8bb42bdec2719de74183d7528  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
82f6b7a102adf2e926e5f1546ef0262f699f92c4c25313be97f745c1014d29fe  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
8d548975b7c25a3adfbdcec355b1e48d351818a5e850d4c35c215c3badbe0b2e  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
ce9279f19bb5219fbd709501031284832d493740689a2e23c2e984102c0a851c  ./target/x86_64-unknown-linux-musl/release/oak_loader

Reproducibility Index diff:


      		
    

  





        


            

            

              
            

        

      


      

            
  

    
  
    
    

  

  



    












      

  
          

  

    
  


  

      

  This was referenced Jun 18, 2020





  


  

      
   Merged

  






      






  


  

      
   Merged

  






      






  


  

      
   Merged

  






  













  
  

    

      

  




    


    

      

              

  
    Sign up for free
    to join this conversation on GitHub.
    Already have an account?
    Sign in to comment


  



      

    

  




  
          




      

  

    
    

    
  

    Reviewers
  


    


        

          

  
      @ipetr0v
  
    ipetr0v



          
            
              
            
          
          ipetr0v approved these changes

        

      

          

  
      @tiziano88
  
    tiziano88


                Awaiting requested review from tiziano88


        
      

      

          

  
      @daviddrysdale
  
    daviddrysdale


                Awaiting requested review from daviddrysdale


        
      


      






    

  


      
  

    Assignees
  



        

    No one assigned







      


  


  

    Labels
  



    

      

    cla: yes









      

  

    

        

    Projects
  


        




    None yet




  



      


  

    
  

    Milestone
  


      No milestone





    
      
          


  

    

      
        

          
    

    Development
  



            


Successfully merging this pull request may close these issues.



  


      
    

  




      

    

  
      

  

    

      4 participants
    

    

        
          @jul-sh 
        
          @daviddrysdale 
        
          @ipetr0v 
        
          @googlebot